feat(thruster): support bottom propeller for single direction mode - #1010
feat(thruster): support bottom propeller for single direction mode#1010tibnor wants to merge 3 commits into
Conversation
When `singleDirection` was enabled the bottom propeller was positioned at the bottom of a (non-existent) downward bar and ended up outside the viewBox, so it could not be used. Attach the bottom propeller to the zero line (the base of the single-direction bar) like the half-size variant does, and widen the viewBox downwards when a bottom propeller is present so it stays in view together with the doubled-height bar and top propeller. Add a `SingleDirectionWithPropellers` story showing a single direction thruster with both a top and a bottom propeller. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ej77SRHRBTnWQtbPBYPhzd
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Storybook example for a single-direction thruster with explicit propeller types, and updates the thruster SVG layout logic to account for bottom propeller presence and half-height bottom propeller rendering. ChangesThruster single-direction layout
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/openbridge-webcomponents/src/navigation-instruments/thruster/thruster.ts (1)
760-773: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove inline explanatory comments from this TypeScript block
Please replace these inline comments with self-explanatory naming (or keep explanation in docs/story), since repo rules disallow inline JS/TS comments except extremely unusual cases.
As per coding guidelines, “Do not include comments in code whatsoever unless the code is extremely unusual and impossible to understand without explanation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openbridge-webcomponents/src/navigation-instruments/thruster/thruster.ts` around lines 760 - 773, Remove the inline explanatory comments from the thruster rendering logic in the thruster module and rely on clearer naming or external documentation instead; specifically, delete the comments around the viewBox/y adjustment and the single-direction thruster bar/propeller placement near the topPropeller and bottomPropeller setup, while keeping the code behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/openbridge-webcomponents/src/navigation-instruments/thruster/thruster.stories.ts`:
- Around line 105-115: The new Storybook export SingleDirectionWithPropellers is
missing the required tags field. Update the story definition in
thruster.stories.ts to include the appropriate tags array per the project
guidelines, matching the pattern used by other story exports in this file or
elsewhere (for example autodocs-related or skip-test tags as applicable).
---
Nitpick comments:
In
`@packages/openbridge-webcomponents/src/navigation-instruments/thruster/thruster.ts`:
- Around line 760-773: Remove the inline explanatory comments from the thruster
rendering logic in the thruster module and rely on clearer naming or external
documentation instead; specifically, delete the comments around the viewBox/y
adjustment and the single-direction thruster bar/propeller placement near the
topPropeller and bottomPropeller setup, while keeping the code behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 261fa2ec-3795-4270-8db0-6f526fd75f9d
📒 Files selected for processing (2)
packages/openbridge-webcomponents/src/navigation-instruments/thruster/thruster.stories.tspackages/openbridge-webcomponents/src/navigation-instruments/thruster/thruster.ts
|
The demo can be viewed at https://openbridge-next-demo--1010-feat-thruster-support-botto-u2f8mp00.web.app |
|
The storybook can be viewed at https://openbridge-next-storybook--1010-feat-thruster-support-ksxwgxr7.web.app |
Add the linux visual-regression baseline for the new `SingleDirectionWithPropellers` story (generated in the same Playwright Docker image CI uses) so the storybook visual test passes. Also drop the inline explanatory comments added in the previous commit to follow the repository guideline of not using inline comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ej77SRHRBTnWQtbPBYPhzd
The single direction viewBox was raised to make room for the bottom propeller, but that clipped the taller bar/arrow used when there is no top propeller (e.g. topPropeller=none with bottomPropeller=single). Keep the top edge at -300 (which fits the tallest bar) and instead extend the viewBox downwards so the bottom propeller fits in every top/bottom propeller combination. Regenerate the SingleDirectionWithPropellers visual baseline accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ej77SRHRBTnWQtbPBYPhzd
Summary
Fixed the viewBox calculation and bottom propeller positioning in single direction thruster mode to properly accommodate both top and bottom propellers while maintaining correct visual layout.
Key Changes
singleDirectionis enabled with a bottom propeller, the viewBox is now adjusted to-80 -268 160 320(instead of-80 -300 160 320) to provide appropriate space for the bottom propeller while keeping the doubled-height bar and top propeller visible-320to-288for the adjusted viewBox0.5scale when eithersingleDirectionHalfSizeORsingleDirectionis enabled, ensuring proper positioning at the zero line where the bar base sits in single direction modeSingleDirectionWithPropellersstory to demonstrate and test single direction thruster with both top and bottom propellersImplementation Details
The fix recognizes that in single direction mode, the bar only extends upward with its base at the zero line. Therefore, the bottom propeller must be attached at the zero line (like the half-size variant) rather than at the bottom of a missing downward bar. The viewBox adjustment ensures sufficient vertical space is allocated below the zero line to display the bottom propeller without clipping.
https://claude.ai/code/session_01Ej77SRHRBTnWQtbPBYPhzd
Summary by CodeRabbit